Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

grpcbox-stream: Fix to support multiple simultaneous unary/strem calls #34

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

vasu-dasari
Copy link
Contributor

@vasu-dasari vasu-dasari commented Nov 16, 2020

grpcbox-stream: Fix to support multiple simultaneous unary/strem calls

This commit addresses stress-test failure mentioned in #29

Added two new APIs:
add_channel(Name, Endpoints, Options)
delete_channel(Pid)

This would give ability to user to add and delete channels on the fly.

Also modified stress_test test case to use this logic. With out this change, stress test fails around 10 simultaneous connections. With this change I can see around 90 simultaneous connections.

Added a stress_test to stress number of concurrent gRPC sessions that can be run. BY default it is set to 10. But can be modified by environment variable GRPCBOX_STRESS_TEST

# To run 25 concurrent sessions:
$ GRPCBOX_STRESS_TEST=25 rebar3 ct --suite grpcbox_SUITE --case stress_test

On my system, it above command breaks around 30-35.

Signed-off-by: Vasu Dasari <[email protected]>
Say there is a rRPC server offering "Streaming Output" service. And when client disconnects server side does not know about the client disconnection. This API offers a way for server to know if client socket is alive or not.

Signed-off-by: Vasu Dasari <[email protected]>
This commit addresses stress-test failure mentioned in tsloughter#29.

Added two new APIs:
add_channel(Name, Endpoints, Options)
delete_channel(Pid)

This would give ability to user to add and delete channels on the fly.

Also modified stress_test test case to use this logic. With out this change, stress test fails around 10 simultaneous connections. With this change I can see around 90 simultaneous connections.

Signed-off-by: Vasu Dasari <[email protected]>
@tsloughter
Copy link
Owner

Hey, sorry I hadn't gotten to this yet. I'll be taking some time this week to work on grpcbox, so will get to it sometime in the next couple days.

@tsloughter
Copy link
Owner

So the solution might be to have a configuration setting for how many connections to open per subchannel. A single connection can handle multiple concurrent streams but maybe you were hitting a limit on that concurrency and the need to increase the connections. But it could also be a chatterbox issue slowing down stream concurrency within a channel, so I also need to check that now.

Base automatically changed from master to main March 22, 2021 19:49
@belltoy
Copy link

belltoy commented Dec 22, 2021

So the solution might be to have a configuration setting for how many connections to open per subchannel.

+1 any progress?

@vasu-dasari
Copy link
Contributor Author

vasu-dasari commented Dec 23, 2021

Hi @tsloughter,

Sorry did not see your comments.
The problem with having an environmental variable to specify number of connections is a subjective one. We might hit a snag at some point. But with this patch, if the user knows by design how the connections are being established then it is better to give control to the application with channel management.
In the patch there is a ct test to demonstrate the problem and working solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants